Conversation
🦋 Changeset detectedLatest commit: 7ccbfb5 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR adds an object-form loader API with an optional Changes
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
View your CI Pipeline Execution ↗ for commit 7ccbfb5
☁️ Nx Cloud last updated this comment at |
238d821 to
7a60354
Compare
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/route.ts (1)
1044-1070:⚠️ Potential issue | 🟡 Minor
BaseRoute.updateLoader()still rejects the new loader object shape.This widens
RouteOptions.loaderto accept{ handler, staleReloadMode }, butBaseRoute.updateLoader()later in this file is still typed asRouteLoaderFnonly. That leaves the imperative path requiring a cast for the new feature instead of accepting it natively.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/route.ts` around lines 1044 - 1070, BaseRoute.updateLoader() is still typed to accept only RouteLoaderFn which rejects the new object-shape loader; change updateLoader's parameter type to the same union used for the loader property (i.e. Constrain<TLoaderFn, RouteLoaderFn<...> | RouteLoaderObject<...>> or the shared alias used for RouteOptions.loader) so it natively accepts both function and object loaders; update any related generics on BaseRoute.updateLoader to match TLoaderDeps/TLoaderFn/TRegister/etc. and adjust internal handling to accept the object shape where currently only RouteLoaderFn is assumed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/poor-dryers-stare.md:
- Around line 1-3: The changeset only bumps '@tanstack/router-core' but the PR
also changes the public file-route loader API in
packages/react-router/src/fileRoute.ts (the new object-form loader typing), so
add a corresponding changeset entry for '@tanstack/react-router' describing this
API change; update .changeset to include a minor (or appropriate) release for
'@tanstack/react-router' and reference the file-route loader typing change so
consumers get the published typing update.
---
Outside diff comments:
In `@packages/router-core/src/route.ts`:
- Around line 1044-1070: BaseRoute.updateLoader() is still typed to accept only
RouteLoaderFn which rejects the new object-shape loader; change updateLoader's
parameter type to the same union used for the loader property (i.e.
Constrain<TLoaderFn, RouteLoaderFn<...> | RouteLoaderObject<...>> or the shared
alias used for RouteOptions.loader) so it natively accepts both function and
object loaders; update any related generics on BaseRoute.updateLoader to match
TLoaderDeps/TLoaderFn/TRegister/etc. and adjust internal handling to accept the
object shape where currently only RouteLoaderFn is assumed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8bf373f-f4bb-4e4b-8cb9-8b56979c5573
📒 Files selected for processing (10)
.changeset/poor-dryers-stare.mddocs/router/api/router/RouteOptionsType.mddocs/router/api/router/RouterOptionsType.mddocs/router/guide/data-loading.mdpackages/react-router/src/fileRoute.tspackages/router-core/src/index.tspackages/router-core/src/load-matches.tspackages/router-core/src/route.tspackages/router-core/src/router.tspackages/router-core/tests/load.test.ts
| --- | ||
| '@tanstack/router-core': minor | ||
| --- |
There was a problem hiding this comment.
Add a @tanstack/react-router changeset here too.
This PR also changes the public file-route loader API in packages/react-router/src/fileRoute.ts. Releasing only @tanstack/router-core would leave @tanstack/react-router consumers without a published version that includes the new object-form loader typing.
Suggested changeset update
---
'@tanstack/router-core': minor
+'@tanstack/react-router': minor
---📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| --- | |
| '@tanstack/router-core': minor | |
| --- | |
| --- | |
| '@tanstack/router-core': minor | |
| '@tanstack/react-router': minor | |
| --- |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/poor-dryers-stare.md around lines 1 - 3, The changeset only bumps
'@tanstack/router-core' but the PR also changes the public file-route loader API
in packages/react-router/src/fileRoute.ts (the new object-form loader typing),
so add a corresponding changeset entry for '@tanstack/react-router' describing
this API change; update .changeset to include a minor (or appropriate) release
for '@tanstack/react-router' and reference the file-route loader typing change
so consumers get the published typing update.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.changeset/poor-dryers-stare.md (1)
1-5:⚠️ Potential issue | 🟠 MajorRelease the framework router packages too.
This expands the earlier
@tanstack/react-routerchangeset concern: the same public typing change also lands inpackages/solid-router/src/fileRoute.tsandpackages/vue-router/src/fileRoute.ts. If the@tanstack/*-coreentries are intentional, please add@tanstack/react-router,@tanstack/solid-router, and@tanstack/vue-routertoo so the newRouteLoaderEntryAPI ships to wrapper consumers.Suggested changeset update
--- '@tanstack/router-core': minor '@tanstack/react-core': minor '@tanstack/solid-core': minor '@tanstack/vue-core': minor +'@tanstack/react-router': minor +'@tanstack/solid-router': minor +'@tanstack/vue-router': minor ---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/poor-dryers-stare.md around lines 1 - 5, The changeset currently bumps '@tanstack/router-core', '@tanstack/react-core', '@tanstack/solid-core', and '@tanstack/vue-core' but omits the corresponding wrapper packages that also changed typing; update the changeset to include '@tanstack/react-router', '@tanstack/solid-router', and '@tanstack/vue-router' with the same "minor" release so the new RouteLoaderEntry API (the public typing change referenced) is published to wrapper consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.changeset/poor-dryers-stare.md:
- Around line 1-5: The changeset currently bumps '@tanstack/router-core',
'@tanstack/react-core', '@tanstack/solid-core', and '@tanstack/vue-core' but
omits the corresponding wrapper packages that also changed typing; update the
changeset to include '@tanstack/react-router', '@tanstack/solid-router', and
'@tanstack/vue-router' with the same "minor" release so the new RouteLoaderEntry
API (the public typing change referenced) is published to wrapper consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46f957b8-e085-4890-9365-6bb8324090a1
📒 Files selected for processing (3)
.changeset/poor-dryers-stare.mdpackages/solid-router/src/fileRoute.tspackages/vue-router/src/fileRoute.ts
Summary by CodeRabbit
New Features
Documentation
Tests